Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Fix: major revision to valid and invalid numeric literal separator "sibling" characters #745

Merged
merged 3 commits into from
Sep 29, 2017

Conversation

rwaldron
Copy link
Contributor

@rwaldron rwaldron commented Sep 28, 2017

Q A
Bug fix? yes
Breaking change? yes
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
License MIT

As a result of testing the Babel transform against NLS tests pending for test262, I discovered a pretty serious oversight in my original parsing. This addresses that issue and adds a substantial amount of tests.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use allowedNumericSeparatorSlibings instead of forbiddenNumericSeparatorSlibings when checking if the caracter before the separator is valid?

bin: [
// 0 - 1
48, 49,
],
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would like to avoid repeating all the numbers... What do you think about doing something like this?

const allowedNumericSeparatorSiblings = {};
allowedNumericSeparatorSiblings.bin = [ 48, 49 ];
allowedNumericSeparatorSiblings.oct = [
  ...allowedNumericSeparatorSiblings.bin, 50, 51, 52, 53, 54, 55,
];
allowedNumericSeparatorSiblings.dec = [
  ...allowedNumericSeparatorSiblings.oct, 56, 57,
];

It wouldn't have a too big performance impact, since they would be computed only when babylon is loaded.
Anyway, It's up to you.

Copy link
Contributor Author

@rwaldron rwaldron Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that.

@rwaldron
Copy link
Contributor Author

Looks like I have to update to use indexOf!

@nicolo-ribaudo
Copy link
Member

@rwaldron Can you fix the linting errors? npm/yarn run lint -- --fix

@hzoo
Copy link
Member

hzoo commented Sep 29, 2017

Looks nice the way it is, but prettier wants the newlines..

@hzoo hzoo merged commit 18c6b4e into babel:master Sep 29, 2017
@hzoo
Copy link
Member

hzoo commented Sep 29, 2017

Thanks @rwaldron!!

@rwaldron
Copy link
Contributor Author

rwaldron commented Oct 2, 2017

Sorry I missed these follow-up messages, I've been afk since Thursday evening.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants